Skip to content

fix(cli): overflow-safe ZIP bounds check; env-bash shebangs#812

Merged
DeusData merged 3 commits into
mainfrom
distill/cli-hygiene
Jul 4, 2026
Merged

fix(cli): overflow-safe ZIP bounds check; env-bash shebangs#812
DeusData merged 3 commits into
mainfrom
distill/cli-hygiene

Conversation

@DeusData

@DeusData DeusData commented Jul 3, 2026

Copy link
Copy Markdown
Owner

distill: overflow-safe ZIP bounds check (#784) + env-bash shebangs (#674)

Two independent review distills sharing src/cli/cli.c, kept as two separate commits on one branch to avoid conflicts.

Commit 1 — 7b93210 fix(cli): overflow-safe ZIP entry bounds check in self-update extraction

Distilled from #784 (thanks @SS-42, co-authored). Refs #784-review.

The real fix (title, not buried): the self-update ZIP truncation check

if (header_end + (int)comp_size > data_len) { break; }

is bypassable for comp_size >= 2^31: the int cast turns the size negative, the sum drops below data_len, and the entry is accepted — authorizing inflate to read up to ~4 GB past the downloaded archive buffer (strm.avail_in = comp_size). Replaced with the overflow-safe, strictly tighter

if (header_end > data_len || comp_size > (uint32_t)(data_len - header_end)) { break; }

Regression guard (red-first, verified via temporary revert of the check):
cli_extract_binary_from_zip_rejects_truncated_deflate_size_over_int_max builds a 52-byte archive whose only entry claims comp_size = 0xFFFF0000 (DEFLATE, uncomp_size = 1, self-terminating 3-byte stream).

  • RED on the old check (verbatim):
    cli_extract_binary_from_zip_rejects_truncated_deflate_size_over_int_max  FAIL tests/test_cli.c:1399: accepted a truncated deflated zip entry with a wrapped compressed size
    
    Pre-fix failure mode: the negative-int bypass admits the entry, zlib is handed avail_in = 0xFFFF0000 (licensed to read ~4 GB past the buffer); the fixture's DEFLATE stream self-terminates, so extraction succeeds and returns non-NULL — a deterministic assertion failure rather than an ASan-dependent crash, in both directions: the fixed check rejects the entry (NULL), the old one accepts it (non-NULL).
  • GREEN with the fix: cli + cypher suites 258 passed, 0 failed.

Secondary cppcheck cleanups from the same review (audited behavior-neutral):

  • zip_read_u16le/zip_read_u32le little-endian helpers replacing the inline shift pyramids (behavior-identical).
  • zip_extract_entry sizes widened to size_t with an explicit UINT_MAX guard before the zlib uInt narrowing.
  • cypher.c eval_condition: removed genuinely unreachable IS NULL / IS NOT NULL null guards (resolve_condition_value result is early-returned on NULL just above); cypher_*_is_null tests unchanged and green.

Commit 2 — 50392a4 fix(scripts): use /usr/bin/env bash shebangs (NixOS has no /bin/bash)

Distilled fresh from #674 (thanks @SuperSandro2000, co-authored). Refs #674-review.

  • Converted #!/bin/bash#!/usr/bin/env bash in the eleven scripts/*.sh holdouts, test-infrastructure/run.sh, and the three Claude Code hook scripts emitted by src/cli/cli.c (gate, session reminder, and the subagent reminder added since Use /usr/bin/env bash for widely compatible shebang #674 was opened).
  • Parser-test coverage preserved (deliberate divergence from Use /usr/bin/env bash for widely compatible shebang #674): the infra_parse_shell* fixtures in tests/test_pipeline.c intentionally keep #!/bin/bash, so absolute-path shebang extraction (r.shebang == "/bin/bash") stays covered. tests/repro fixtures untouched. No test asserts the emitted hook scripts' shebang, so no assertion updates were required.
  • Replaced the GitHub-PAT-shaped fixture string flagged in the Use /usr/bin/env bash for widely compatible shebang #674 thread with ghp_FAKEFAKEFAKEFAKEFAKEFAKEFAKEFAKEFAKE — obviously fake, still ghp_ + 36 alnum so the secret detector (pass_infrascan.c) keeps firing; envscan_secret_value_exclusion green.
  • bash -n clean on all 12 changed scripts.

Verification (after both commits)

  • make -f Makefile.cbm cbm-Werror clean.
  • make -f Makefile.cbm lint-ci — cppcheck, clang-format, NOLINT check all green.
  • test-runner pipeline — 209 passed, 0 failed.
  • test-runner cli cypher — 258 passed, 0 failed.

Refs #784-review, refs #674-review.

DeusData and others added 2 commits July 3, 2026 20:05
The truncation check `header_end + (int)comp_size > data_len` was
bypassable for comp_size >= 2^31: the int cast turns the size negative,
the sum drops below data_len, and the entry is accepted -- authorizing
inflate to read up to ~4GB past the downloaded archive buffer during
self-update ZIP extraction. Replace it with
`header_end > data_len || comp_size > (uint32_t)(data_len - header_end)`,
which is overflow-safe and strictly tighter.

Regression guard cli_extract_binary_from_zip_rejects_truncated_deflate_size_over_int_max
builds a 52-byte archive whose entry claims comp_size=0xFFFF0000 with a
self-terminating DEFLATE stream: the old check admits it and extraction
returns non-NULL (verified red pre-fix); the new check rejects it.

Secondary cppcheck cleanups from the same review:
- cli.c: extract zip_read_u16le/zip_read_u32le little-endian helpers
  (behavior-identical) and widen zip_extract_entry sizes to size_t with
  an explicit UINT_MAX guard before the zlib uInt narrowing.
- cypher.c: drop unreachable IS NULL / IS NOT NULL null guards in
  eval_condition (resolve_condition_value result is checked earlier).

Distilled from PR #784.

Co-authored-by: SS-42 <noreply@incogni.to>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
On NixOS (and other non-FHS systems) /bin/bash does not exist, so
scripts with an absolute shebang fail to run. Switch the remaining
holdouts to /usr/bin/env bash: eleven scripts/*.sh,
test-infrastructure/run.sh, and the three Claude Code hook scripts
emitted by src/cli/cli.c (gate, session reminder, subagent reminder).

Distilled from PR #674, with parser-test coverage preserved: the
infra_parse_shell* fixtures in tests/test_pipeline.c intentionally keep
#!/bin/bash so absolute-path shebang extraction stays covered, and
tests/repro fixtures are untouched.

Also replace the GitHub-PAT-shaped fixture string flagged in the #674
thread with an obviously fake placeholder (ghp_FAKE...) that still
matches the ghp_ + 36-alnum secret detector.

Co-authored-by: Sandro Jäckel <sandro.jaeckel@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData DeusData enabled auto-merge July 3, 2026 22:38
@DeusData DeusData merged commit 010ac9b into main Jul 4, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant